Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: close only assignee PR #10

Merged
merged 24 commits into from
Jul 29, 2024
Merged

fix: close only assignee PR #10

merged 24 commits into from
Jul 29, 2024

Conversation

Keyrxng
Copy link
Member

@Keyrxng Keyrxng commented Jul 15, 2024

Resolves #4

  • adds check for PR closures
  • adds tests

I think there may be legitimate scenarios where a PR exists cross-repo but I'm unsure about cross-org. But that will likely be the exception rather than the rule, how do you feel about this approach?

for await (const pr of linkedPullRequests) {
    if (pr.author !== author || pr.organization !== currentRepo.owner.login || pr.repository !== currentRepo.name) {
      continue;

QA: ubq-testing#2 (comment)

  • I have two opened PRs one by this account, one by my testing account and the timeline contains PRs from this repo. The comment shows only this account's PR being closed.

@github-actions GitHub Actions
Copy link

Unused files (2)

src/main.ts, src/plugin.ts

Unused dependencies (2)

Filename dependencies
package.json @actions/core
@actions/github

Unused exports (1)

Filename exports
src/types/plugin-input.ts startStopSettingsValidator

Unused types (1)

Filename types
src/types/plugin-input.ts PluginInputs

@Keyrxng Keyrxng mentioned this pull request Jul 15, 2024
Merged
@0x4007
Copy link
Member

0x4007 commented Jul 15, 2024 •
edited
Loading

I think there may be legitimate scenarios where a PR exists cross-repo but I'm unsure about cross-org. But that will likely be the exception rather than the rule

Ideally we should have support for all the above scenarios. Cross org indeed is rare. If its too complicated then perhaps we can make a new task for it (if needed in the future.)

@Keyrxng
Copy link
Member Author

Keyrxng commented Jul 16, 2024

Cross org indeed is rare. If its too complicated then perhaps we can make a new task for it (if needed in the future.)

I think a new task if required is the way to go.

Correct me if I'm wrong, but for cross-org closures we'd need the bot to be installed in the other org right? I'm assuming based on the earlier attempts of my bot to close #1 and it never succeeding

@Keyrxng
Copy link
Member Author

Keyrxng commented Jul 16, 2024 •
edited
Loading

I want to add that even with the example in the PR body it would still likely be possible for a PR to be closed in error within the same repo depending on what is being cross-referenced

until it's live-tested I can't be sure how effective it's going to be. We are making assumptions about what is being cross-referenced in production and I know there's a good chance it'll close a PR in error eventually with cross-repo closures.

I was thinking parse each linked PR body and you'd expect to find a Resolves|Requires # which seems like a good source of truth but I don't think it's infallible either

@Keyrxng
Copy link
Member Author

Keyrxng commented Jul 22, 2024

I've added the PR body check

I referenced the issue from another repo from both an issue and PR. And I couldn't find either of those references in the timeline pre-filtering, which is odd right because previously it was showing PRs between this org and mine.

The sooner we see it live QA-d the better and I'm happy to handle any fixes/tweaks that are needed

fresh QA:

ubq-testing#5

gentlementlegen
gentlementlegen reviewed Jul 22, 2024
View reviewed changes
.github/knip.ts Outdated Show resolved Hide resolved
gentlementlegen
gentlementlegen reviewed Jul 22, 2024
View reviewed changes
tests/main.test.ts Outdated Show resolved Hide resolved
@Keyrxng Keyrxng mentioned this pull request Jul 23, 2024
Closed
Keyrxng and others added 2 commits July 23, 2024 01:01
gentlementlegen
gentlementlegen reviewed Jul 26, 2024
View reviewed changes
.github/knip.ts Outdated Show resolved Hide resolved
0x4007
0x4007 reviewed Jul 28, 2024
View reviewed changes
eslint.config.mjs Show resolved Hide resolved
src/handlers/user-start-stop.ts Outdated Show resolved Hide resolved
@0x4007
Copy link
Member

0x4007 commented Jul 28, 2024

After my comments are addressed I'm okay to merge.

@Keyrxng
Copy link
Member Author

Keyrxng commented Jul 29, 2024

just double checking that we are good to merge here

@gentlementlegen
Copy link
Member

@Keyrxng Yes, the merge issue is unrelated to this and will be addressed within the kernel. I am fine with this pull-request.

@Keyrxng Keyrxng merged commit b50f0a8 into ubiquity-os-marketplace:development Jul 29, 2024
2 checks passed
@ubiquibot ubiquibot bot mentioned this pull request Jul 29, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gentlementlegen gentlementlegen gentlementlegen approved these changes

@0x4007 0x4007 0x4007 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Wrong linked PR is closed on /stop
3 participants
@Keyrxng @0x4007 @gentlementlegen